Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bump CSI spec 1.10 #1262

Merged
merged 4 commits into from
Sep 24, 2024
Merged

Bump CSI spec 1.10 #1262

merged 4 commits into from
Sep 24, 2024

Conversation

jsafrane
Copy link
Contributor

@jsafrane jsafrane commented Aug 20, 2024 •
edited
Loading

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Bump CSI spec to 1.10.0 and csi-test to 5.3.0.

  • gomock is not able to match newly generated Protobuf messages from csi-test, because their private fields differ. Implement a custom message matcher.
  • k8s.io/apimachinery can be used to compare API objects, but it panics on the new protobuf messages. Use a generic cmp.Equal instead, as suggested in https://protobuf.dev/reference/go/faq/#deepequal

WIP:

  • the matcher should be probably in csi-tests repo, as it's going to be used by many sidecars.
Updated CSI spec to 1.10.0

cc @AndrewSirenko @xing-yang @gnufied

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 20, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 20, 2024
@jsafrane jsafrane force-pushed the bump-spec-1.10 branch 3 times, most recently from 3f4ede0 to 398f2c2 Compare August 21, 2024 13:18
@jsafrane jsafrane mentioned this pull request Aug 21, 2024
Open
@jsafrane
Copy link
Contributor Author

I filed an issue in the CSI spec repo: container-storage-interface/spec#570
This should not be a minor package bump.

@AndrewSirenko AndrewSirenko mentioned this pull request Aug 21, 2024
Merged
@AndrewSirenko AndrewSirenko mentioned this pull request Aug 28, 2024
Merged
@AndrewSirenko
Copy link
Contributor

AndrewSirenko commented Aug 28, 2024 •
edited
Loading

This fixes tests for external-health-monitor and liveness probe when moved to csi-test

See tests passing with replaced csi-test module:

I can raise a PR on your fork for this provisioner PR, changing Protobuf -> csitestutil.Protobuf once we release new version of csi-test.

@jsafrane jsafrane changed the title WIP: Bump CSI spec 1.10 Bump CSI spec 1.10 Sep 18, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2024
@jsafrane
Bump CSI spec to 1.10.0
1bc306d 
And csi-test to 5.3.0 to get updated mocks.
@jsafrane
Update unit tests to new gRPC
1416604 
gomock cannot compare protobuf messages due to mismatches in private
fields. Use a custom matcher for protobuf messages.
@jsafrane
Fix panic in topology tests
e26da09 
k8s.io/apimachinery can be used to compare API objects, but it panics on
protobuf messages. Use a generic cmp.Equal, with protobuf hints.
@jsafrane
Fix govet errors
d794397 
Do not copy NodeGetInfoResponse, it contains many private fields, incl. a
mutex that should not be copied. Use a pointer instead.
@jsafrane
Copy link
Contributor Author

@AndrewSirenko I updated to csi-test 5.3.1, PTAL

@AndrewSirenko
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2024
@AndrewSirenko
Copy link
Contributor

/test pull-kubernetes-csi-external-provisioner-1-27-on-kubernetes-1-27
/test pull-kubernetes-csi-external-provisioner-1-28-on-kubernetes-1-28

@k8s-ci-robot
Copy link
Contributor

@AndrewSirenko: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test pull-kubernetes-csi-external-provisioner-1-29-on-kubernetes-1-29
  • /test pull-kubernetes-csi-external-provisioner-1-30-on-kubernetes-1-30
  • /test pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-1-26
  • /test pull-kubernetes-csi-external-provisioner-unit

The following commands are available to trigger optional jobs:

  • /test pull-kubernetes-csi-external-provisioner-1-29-on-kubernetes-master
  • /test pull-kubernetes-csi-external-provisioner-1-30-on-kubernetes-master
  • /test pull-kubernetes-csi-external-provisioner-1-31-on-kubernetes-1-31
  • /test pull-kubernetes-csi-external-provisioner-1-31-on-kubernetes-master
  • /test pull-kubernetes-csi-external-provisioner-alpha-1-30-on-kubernetes-1-30
  • /test pull-kubernetes-csi-external-provisioner-canary
  • /test pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-master

Use /test all to run the following jobs that were automatically triggered:

  • pull-kubernetes-csi-external-provisioner-1-29-on-kubernetes-1-29
  • pull-kubernetes-csi-external-provisioner-1-30-on-kubernetes-1-30
  • pull-kubernetes-csi-external-provisioner-1-31-on-kubernetes-1-31
  • pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-1-26
  • pull-kubernetes-csi-external-provisioner-unit

In response to this:

/test pull-kubernetes-csi-external-provisioner-1-27-on-kubernetes-1-27
/test pull-kubernetes-csi-external-provisioner-1-28-on-kubernetes-1-28

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot merged commit 0f8b166 into kubernetes-csi:master Sep 24, 2024
8 checks passed
@xing-yang xing-yang mentioned this pull request Nov 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrewSirenko AndrewSirenko AndrewSirenko left review comments

@gnufied gnufied gnufied left review comments

@ggriffiths ggriffiths Awaiting requested review from ggriffiths

@mauriciopoppe mauriciopoppe Awaiting requested review from mauriciopoppe

Assignees

@AndrewSirenko AndrewSirenko

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jsafrane @k8s-ci-robot @AndrewSirenko @gnufied